Pushhandler #23
Pushhandler #23
Conversation
| @@ -764,7 +767,7 @@ type HeadersFrameParam struct { | |||
| // EndStream indicates that the header block is the last that | |||
| // the endpoint will send for the identified stream. Setting | |||
| // this flag causes the stream to enter one of "half closed" | |||
| // states. | |||
| // states. It is not sent if this write is a push promise | |||
There was a problem hiding this comment.
Minor, but there's no period at the end of this (and many of your other) comment sentence, which isn't consistent.
|
There was a thread on [email protected] with title "Pushing 304's" about pushing HEAD requests. So we should probably make that possible. I can't review this for a couple days, but thanks for starting to tackle it. |
As predicted, I ended up unifying the Headers and Push Promise packing. This is in preparation for reusing the writeResHeaders functionality of sending continuation frames if the headers are too big.
The API uses loopback to the top handler in order to make sure that the resources being pushed are related to the resource being fetched. If the header parameter is nil, we copy the headers from the initiating request. This is mostly a shortcut for the common case where we don't want to specify any new request headers.
DanielMorsing
force-pushed
the
pushhandler
branch
from
1628f09
to
c2f4843
Compare
|
You've missed to implement push counter for streams in the func (sc *serverConn) closeStream(st *stream, err error) {
sc.serveG.check()
if st.state == stateIdle || st.state == stateClosed {
panic(fmt.Sprintf("invariant; can't close stream in state %v", st.state))
}
st.state = stateClosed
sc.curOpenStreams--And in case of 0 we get: 4294967295. And then we try to check it here: func (sc *serverConn) processHeaderBlockFragment(st *stream, frag []byte, end bool) error {
...
if sc.curOpenStreams > sc.advMaxStreams {
// "Endpoints MUST NOT exceed the limit set by their
// peer. An endpoint that receives a HEADERS frame
// that causes their advertised concurrent stream
// limit to be exceeded MUST treat this as a stream
// error (Section 5.4.2) of type PROTOCOL_ERROR or
// REFUSED_STREAM."
if sc.unackedSettings == 0 {
// They should know better.
return StreamError{st.id, ErrCodeProtocol}
}I could not find how to add patch here so I sent new pull request: #34 Thank you. |
|
Sorry about leaving this for so long. Right now there are multiple issues with the code. Along with the curopenstreams issue that you discovered, we also need to take into account the clientMaxStreams variable that limits the amount of push promises that a server can initiate with a client concurrently. I have some free time today, so I'll probably push some new code soon. |
|
Nevermind, I looks like you fixed this independently. I'll wait for Brad to have a look. He's on vacation though, so it might be a while. |
|
This repo just moved into the official Go repo. See https://github.com/bradfitz/http2/blob/master/README for the move details. File new bugs at: https://github.com/golang/go/issues/new?title=x/net/http2:+ Closing this PR as we're now using Gerrit for code review. Please file a bug or send the review to Gerrit if this is still relevant. Sorry for how long this languished here. |
This is the initial implementation of server push. There's still a lot to do. Consider this pull request more of a request for comments on the design rather than final code push.